Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix -with-sources not fetching differently named source packages #1404

Merged
merged 6 commits into from
Dec 11, 2024
Merged

Fix -with-sources not fetching differently named source packages #1404

merged 6 commits into from
Dec 11, 2024

Conversation

schoenherrg
Copy link
Contributor

@schoenherrg schoenherrg commented Dec 9, 2024 •
edited by neolynx
Loading

Fixes #1403

Description of the Change

This fixes a bug where aptly doesn't behave as specified in the documentation.

The core of the change is to just add an option to Filter to indicate that source packages need to be included and, if that option is set, add the missing packages to the result.

--- a/deb/list.go
+++ b/deb/list.go
@@ -524,6 +537,37 @@ func (l *PackageList) Filter(options FilterOptions) (*PackageList, error) {
  for _, query := range options.Queries {
          _ = result.Append(query.Query(l))
  }
+ // The above loop already finds source packages that are named equal to their binary package, but we still need
+ // to account for those that are named differently.
+ if options.WithSources {
+         sourceQueries := make([]PackageQuery, 0)
+         for _, pkg := range result.packages {
+                 // Find sourceName, sourceVersion
+                 // ...
+                 sourceQueries = append(sourceQueries, &DependencyQuery{Dependency{
+                         Pkg:          sourceName,
+                         Version:      sourceVersion,
+                         Relation:     VersionEqual,
+                         Architecture: ArchitectureSource,
+                 }})
+         }
+         for _, query := range sourceQueries {
+                 _ = result.Append(query.Query(l))
+         }
+ }

Note that I also refactored Filter to take a struct of options, because I didn't want to add yet another argument to the already very long argument list.

--- a/deb/list.go
+++ b/deb/list.go

-// Filter filters package index by specified queries (ORed together), possibly pulling dependencies
-func (l *PackageList) Filter(queries []PackageQuery, withDependencies bool, source *PackageList, dependencyOptions int, architecturesList []string) (*PackageList, error) {
-   return l.FilterWithProgress(queries, withDependencies, source, dependencyOptions, architecturesList, nil)
+// FilterOptions specifies options for Filter()
+type FilterOptions struct {
+ Queries           []PackageQuery
+ WithDependencies  bool
+ Source            *PackageList
+ DependencyOptions int
+ Architectures     []string
+ Progress          aptly.Progress // set to non-nil to report progress
 }

-// FilterWithProgress filters package index by specified queries (ORed together), possibly pulling dependencies and displays progress
-func (l *PackageList) FilterWithProgress(queries []PackageQuery, withDependencies bool, source *PackageList, dependencyOptions int, architecturesList []string, progress aptly.Progress) (*PackageList, error) {
+// Filter filters package index by specified queries (ORed together), possibly pulling dependencies
+func (l *PackageList) Filter(options FilterOptions) (*PackageList, error) {

A call to Filter now looks like this (you can leave out arguments that have the default value):

-   repo.packageList, err = repo.packageList.FilterWithProgress([]PackageQuery{filterQuery}, repo.FilterWithDeps, emptyList, dependencyOptions, repo.Architectures, progress)
+ repo.packageList, err = repo.packageList.Filter(FilterOptions{
+         Queries:           []PackageQuery{filterQuery},
+         WithDependencies:  repo.FilterWithDeps,
+         Source:            emptyList,
+         DependencyOptions: dependencyOptions,
+         Architectures:     repo.Architectures,
+         Progress:          progress,
+ })

This change is independent of the bugfix though and I can revert it if you don't like it.

Checklist

  • unit-test added (if change is algorithm)
  • functional test added/updated (if change is functional)
  • author name in AUTHORS

@schoenherrg
Refactor Filter options into a struct
3b785e4 
It was already a lot of options for one method and I am going to add
another one in the next commit.
@schoenherrg
Fix -with-sources not downloading differently named sources
0c76677 
Such as e.g. downloading 'glibc' when the sources for 'libc6'
are requested.
@schoenherrg
Add unit tests for filtering with source packages
ef68152
@schoenherrg
Copy link
Contributor Author

For the test PublishSnapshot41Test, I am not sure what to do. The mirror that this test is using (http://repo.aptly.info/system-tests/security.debian.org/debian-security) seems to not contain the required source packages. This was previously not an issue because not Aptly was just ignoring those source packages even though the flag -with-sources is specified. Should I change the test and remove the -with-sources flag?

    ERROR: unable to update: download errors:
      HTTP code 404 while fetching http://repo.aptly.info/system-tests/security.debian.org/debian-security/pool/updates/main/u/util-linux/util-linux_2.33.1-0.1+deb10u1.dsc
      HTTP code 404 while fetching http://repo.aptly.info/system-tests/security.debian.org/debian-security/pool/updates/main/u/util-linux/util-linux_2.33.1-0.1+deb10u1.debian.tar.xz
      HTTP code 404 while fetching http://repo.aptly.info/system-tests/security.debian.org/debian-security/pool/updates/main/u/util-linux/util-linux_2.33.1.orig.tar.xz
      HTTP code 404 while fetching http://repo.aptly.info/system-tests/security.debian.org/debian-security/pool/updates/main/g/glibc/glibc_2.28-10+deb10u2.debian.tar.xz
      HTTP code 404 while fetching http://repo.aptly.info/system-tests/security.debian.org/debian-security/pool/updates/main/g/glibc/glibc_2.28-10+deb10u2.dsc
      HTTP code 404 while fetching http://repo.aptly.info/system-tests/security.debian.org/debian-security/pool/updates/main/g/glibc/glibc_2.28.orig.tar.xz
      HTTP code 404 while fetching http://repo.aptly.info/system-tests/security.debian.org/debian-security/pool/updates/main/n/ncurses/ncurses_6.1+20181013-2+deb10u5.debian.tar.xz
      HTTP code 404 while fetching http://repo.aptly.info/system-tests/security.debian.org/debian-security/pool/updates/main/n/ncurses/ncurses_6.1+20181013-2+deb10u5.dsc
      HTTP code 404 while fetching http://repo.aptly.info/system-tests/security.debian.org/debian-security/pool/updates/main/n/ncurses/ncurses_6.1+20181013.orig.tar.gz
      HTTP code 404 while fetching http://repo.aptly.info/system-tests/security.debian.org/debian-security/pool/updates/main/z/zlib/zlib_1.2.11.dfsg-1+deb10u2.debian.tar.xz
      HTTP code 404 while fetching http://repo.aptly.info/system-tests/security.debian.org/debian-security/pool/updates/main/z/zlib/zlib_1.2.11.dfsg-1+deb10u2.dsc
      HTTP code 404 while fetching http://repo.aptly.info/system-tests/security.debian.org/debian-security/pool/updates/main/z/zlib/zlib_1.2.11.dfsg.orig.tar.gz
      HTTP code 404 while fetching http://repo.aptly.info/system-tests/security.debian.org/debian-security/pool/updates/main/x/xz-utils/xz-utils_5.2.4-1+deb10u1.debian.tar.xz
      HTTP code 404 while fetching http://repo.aptly.info/system-tests/security.debian.org/debian-security/pool/updates/main/x/xz-utils/xz-utils_5.2.4-1+deb10u1.dsc
      HTTP code 404 while fetching http://repo.aptly.info/system-tests/security.debian.org/debian-security/pool/updates/main/x/xz-utils/xz-utils_5.2.4.orig.tar.xz
      HTTP code 404 while fetching http://repo.aptly.info/system-tests/security.debian.org/debian-security/pool/updates/main/e/e2fsprogs/e2fsprogs_1.44.5-1+deb10u2.debian.tar.xz
      HTTP code 404 while fetching http://repo.aptly.info/system-tests/security.debian.org/debian-security/pool/updates/main/e/e2fsprogs/e2fsprogs_1.44.5-1+deb10u2.dsc
      HTTP code 404 while fetching http://repo.aptly.info/system-tests/security.debian.org/debian-security/pool/updates/main/e/e2fsprogs/e2fsprogs_1.44.5.orig.tar.gz
      HTTP code 404 while fetching http://repo.aptly.info/system-tests/security.debian.org/debian-security/pool/updates/main/e/e2fsprogs/e2fsprogs_1.44.5.orig.tar.gz.asc

@5hir0kur0
Copy link
Contributor

5hir0kur0 commented Dec 9, 2024 •
edited
Loading

@neolynx this PR is actually from me. I was told to contribute this with an @siemens.com email/account because I'm doing this for Siemens and they want more visibility for the open source contributions that they paid for.

@neolynx
Copy link
Member

neolynx commented Dec 9, 2024

the test is failing due to missing files in the S3 mirror for system tests.

I will add those..

@neolynx
Copy link
Member

neolynx commented Dec 9, 2024 •
edited
Loading

additional files added, test are green now :)

codecov/patch — 90.59% of diff hit (target 74.81%)

nice job 👍

@neolynx neolynx added the needs review Ready for review & merge label Dec 9, 2024
@schoenherrg
Fix failing system test
8c3fe8d 
The fix of the -with-filter flag causes the following previously
missing source files to be downloaded, so I updated the test file.

```
rkward_0.7.5-1~bullseyecran.0.debian.tar.xz
rkward_0.7.5-1~bullseyecran.0.dsc
rkward_0.7.5.orig.tar.gz
rpy2_3.5.12-1~bullseyecran.0.debian.tar.xz
rpy2_3.5.12-1~bullseyecran.0.dsc
rpy2_3.5.12.orig.tar.gz
```
@schoenherrg schoenherrg force-pushed the fix/with-sources-ignored branch from 32e458c to 8c3fe8d Compare December 10, 2024 02:53
@5hir0kur0
Copy link
Contributor

additional files added, test are green now :)

codecov/patch — 90.59% of diff hit (target 74.81%)

nice job 👍

Thank you 😊

@neolynx
Copy link
Member

neolynx commented Dec 11, 2024

@schoenherrg could you provide me access to the PR ?

I wanted to push an update on the system test, checking for the additional source pkgs with different name explicitely...

@neolynx neolynx merged commit 93650ef into aptly-dev:master Dec 11, 2024
41 checks passed
@neolynx neolynx deleted the fix/with-sources-ignored branch December 11, 2024 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szakalboss szakalboss szakalboss approved these changes

@5hir0kur0 5hir0kur0 5hir0kur0 approved these changes

@neolynx neolynx Awaiting requested review from neolynx

Assignees

@5hir0kur0 5hir0kur0

Labels
needs review Ready for review & merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sources named differently than binary package not included with -with-sources
4 participants
@schoenherrg @5hir0kur0 @neolynx @szakalboss